Let the force field resolve the partial charge assignment - #2152
Conversation
|
No API break detected ✅ Griffe output |
| if force_field is not None: | ||
| ff = ForceField(force_field) | ||
| if "ToolkitAM1BCC" in ff.registered_parameter_handlers: | ||
| ff.deregister_parameter_handler("ToolkitAM1BCC") |
There was a problem hiding this comment.
If the user has 2.3.0 this will apply NAGL charges as the force field defines but might not be what the user wants if they expect am1bcc
There was a problem hiding this comment.
Not sure I understand what you mean here sorry.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2152 +/- ##
==========================================
- Coverage 95.14% 90.44% -4.70%
==========================================
Files 213 213
Lines 20916 20927 +11
==========================================
- Hits 19900 18927 -973
- Misses 1016 2000 +984
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| RDKitToolkitWrapper, | ||
| ) | ||
| from openff.units import unit | ||
| from sympy.codegen.ast import continue_ |
| def assign_offmol_partial_charges( | ||
| offmol: OFFMol, | ||
| overwrite: bool, | ||
| method: Literal["am1bcc", "am1bccelf10", "nagl", "espaloma"], |
There was a problem hiding this comment.
I liked your idea yesterday of having a "forcefield" entry to make it clear that this is the method that was being applied.
| toolkit_backend: Literal["ambertools", "openeye", "rdkit"], | ||
| generate_n_conformers: int | None, | ||
| nagl_model: str | None, | ||
| force_field: list[str] | None = None, |
There was a problem hiding this comment.
| force_field: list[str] | None = None, | |
| force_fields: list[str] | None = None, |
Should it be plural?
| if force_field is not None: | ||
| ff = ForceField(force_field) | ||
| if "ToolkitAM1BCC" in ff.registered_parameter_handlers: | ||
| ff.deregister_parameter_handler("ToolkitAM1BCC") |
There was a problem hiding this comment.
Not sure I understand what you mean here sorry.
2 participants
Fixes #2116, #2117
LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes / no
If yes, please provide details here:
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).
Developers certificate of origin